Skip to content

ARROW-12669:[C++][Python] Implement a new scalar function: list_element#11159

Closed
aucahuasi wants to merge 1 commit intoapache:masterfrom
aucahuasi:return-array-for-each-list-in-array
Closed

ARROW-12669:[C++][Python] Implement a new scalar function: list_element#11159
aucahuasi wants to merge 1 commit intoapache:masterfrom
aucahuasi:return-array-for-each-list-in-array

Conversation

@aucahuasi
Copy link
Contributor

@aucahuasi aucahuasi commented Sep 14, 2021

@github-actions
Copy link

github-actions bot commented Sep 14, 2021

https://issues.apache.org/jira/browse/ARROW-12669

@aucahuasi
Copy link
Contributor Author

aucahuasi commented Sep 14, 2021

Tomorrow I'll work on the python and R bindings for this new function
cc @nealrichardson @edponce @ianmcook

@aucahuasi aucahuasi changed the title ARROW-12669:[C++][Python][R] Implement a new vector function: list_element ARROW-12669:[C++][R] Implement a new vector function: list_element Sep 14, 2021
@aucahuasi aucahuasi force-pushed the return-array-for-each-list-in-array branch from 91f1638 to b38ee7e Compare September 15, 2021 01:14
@aucahuasi aucahuasi marked this pull request as ready for review September 15, 2021 01:19
@aucahuasi aucahuasi changed the title ARROW-12669:[C++][R] Implement a new vector function: list_element ARROW-12669:[C++][Python] Implement a new vector function: list_element Sep 15, 2021
@lidavidm
Copy link
Member

lidavidm commented Sep 15, 2021

Just a quick thought, why is this a vector kernel instead of a scalar one? (Though with one input it's maybe not a big difference.)

@lidavidm
Copy link
Member

lidavidm commented Sep 15, 2021

Oh wait nevermind, the index here is a parameter and not an options.

lidavidm
lidavidm reviewed Sep 15, 2021
View reviewed changes
@nealrichardson
Copy link
Member

nealrichardson commented Sep 15, 2021

why is this a vector kernel instead of a scalar one?

I think it has to be a scalar kernel if we want to be able to call it in a projection node, which is why we're doing this now--right?

@lidavidm
Copy link
Member

lidavidm commented Sep 15, 2021

why is this a vector kernel instead of a scalar one?

I think it has to be a scalar kernel if we want to be able to call it in a projection node, which is why we're doing this now--right?

Ah, right - plus expressions aren't evaluatable unless they're scalar:

arrow/cpp/src/arrow/compute/exec/expression.cc

Lines 503 to 506 in 9cf2372

if (!expr.IsScalarExpression()) {
return Status::Invalid(
"ExecuteScalarExpression cannot Execute non-scalar expression ", expr.ToString());
}

I think most kernels like this pass the second argument via options, not as an argument. I was also debating whether to comment about this, but might as well: there's no need to generate kernels for different index types, especially if we just make the index an option instead of a parameter. (It'll also limit the number of special cases to account for in type coverage testing.)

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Sep 15, 2021

I think most kernels like this pass the second argument via options, not as an argument.

Why can't this be a scalar kernel while still having index as a second argument?
Having the index as argument keeps the flexibility to be able to get a different index of each row (I don't know if this would be an important functionality though, but there are probably use cases)

ianmcook
ianmcook reviewed Sep 15, 2021
View reviewed changes
@lidavidm
Copy link
Member

lidavidm commented Sep 15, 2021

I think most kernels like this pass the second argument via options, not as an argument.

Why can't this be a scalar kernel while still having index as a second argument?
Having the index as argument keeps the flexibility to be able to get a different index of each row (I don't know if this would be an important functionality though, but there are probably use cases)

Ah, you're right. So this should just be a scalar kernel, then.

@aucahuasi aucahuasi force-pushed the return-array-for-each-list-in-array branch 2 times, most recently from ea560fa to 954ca3e Compare September 16, 2021 01:55
edponce
edponce suggested changes Sep 16, 2021
View reviewed changes
edponce
edponce suggested changes Sep 16, 2021
View reviewed changes
edponce
edponce suggested changes Sep 16, 2021
View reviewed changes
lidavidm
lidavidm reviewed Sep 16, 2021
View reviewed changes
@ianmcook ianmcook mentioned this pull request Sep 17, 2021
Closed
@aucahuasi aucahuasi force-pushed the return-array-for-each-list-in-array branch from 954ca3e to 6201ebf Compare September 18, 2021 00:05
lidavidm
lidavidm reviewed Sep 18, 2021
View reviewed changes
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks good to me, I just left one note - the scalar kernel doesn't need to be templated on the list type, so you can save yourself some generated code there.

@aucahuasi aucahuasi force-pushed the return-array-for-each-list-in-array branch from 6201ebf to 2a207d5 Compare September 18, 2021 14:57
edponce
edponce suggested changes Sep 18, 2021
View reviewed changes
@ianmcook ianmcook changed the title ARROW-12669:[C++][Python] Implement a new vector function: list_element ARROW-12669:[C++][Python] Implement a new scalar function: list_element Sep 18, 2021
@aucahuasi aucahuasi force-pushed the return-array-for-each-list-in-array branch from 2a207d5 to 5b899d6 Compare September 20, 2021 17:01
edponce
edponce reviewed Sep 20, 2021
View reviewed changes
lidavidm
lidavidm approved these changes Sep 20, 2021
View reviewed changes
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

edponce
edponce reviewed Sep 20, 2021
View reviewed changes
edponce
edponce reviewed Sep 20, 2021
View reviewed changes
@aucahuasi
implement new vector function: list_element ARROW-12669
2369457 
add python test and C++ docs for the new function: list_element

convert list_element into a scalar function

minor changes

format

support scalar inputs for list_element, improve tests

minor changes

less generated code thanks to some template tricks

less generated code, again using template tricks
@aucahuasi aucahuasi force-pushed the return-array-for-each-list-in-array branch from 5b899d6 to 2369457 Compare September 20, 2021 18:10
edponce
edponce suggested changes Sep 20, 2021
View reviewed changes
edponce
edponce approved these changes Sep 20, 2021
View reviewed changes
Copy link
Contributor

@edponce edponce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for resolving this issue!

@lidavidm lidavidm closed this in bdb2f74 Sep 20, 2021
@aucahuasi aucahuasi mentioned this pull request Oct 4, 2021
Closed
ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
@aucahuasi @ViniciusSouzaRoque
ARROW-12669:[C++][Python] Implement a new scalar function: list_element
c6ba963 
https://issues.apache.org/jira/browse/ARROW-12669

Closes apache#11159 from aucahuasi/return-array-for-each-list-in-array

Authored-by: Percy Camilo Triveño Aucahuasi <percy.camilo.ta@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
@asfimport asfimport mentioned this pull request Sep 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ianmcook ianmcook ianmcook left review comments

@lidavidm lidavidm lidavidm approved these changes

+1 more reviewer

@edponce edponce edponce approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Component: C++ Component: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@aucahuasi @lidavidm @nealrichardson @jorisvandenbossche @ianmcook @edponce